Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactored evp sub cycling loop #756

Merged
merged 2 commits into from
Aug 24, 2022
Merged

Refactored evp sub cycling loop #756

merged 2 commits into from
Aug 24, 2022

Conversation

TillRasmussen
Copy link
Contributor

@TillRasmussen TillRasmussen commented Aug 22, 2022 •
edited
Loading

For detailed information about submitting Pull Requests (PRs) to the CICE-Consortium,
please refer to: https://github.com/CICE-Consortium/About-Us/wiki/Resource-Index#information-for-developers

PR checklist

  • Refactored the evp subcycling in order to cycle for each grid (B, C and CD)
    ENTER INFORMATION HERE

  • [] Developer(s): @TillRasmussen
    ENTER INFORMATION HERE

  • Suggest PR reviewers from list in the column to the right. @apcraig @eclare108213 @phil-blain

  • Please copy the PR test results link or provide a summary of testing completed below.

  • How much do the PR code changes differ from the unmodified code?

    • [x ] bit for bit
    • different at roundoff level
    • more substantial

  • Does this PR create or have dependencies on Icepack or any other models?

    • Yes
    • [ x] No

  • Does this PR add any new test cases?
    refactorbaseCD.log
    refactorbasetB.log
    refactorbasetC.log
    refactortestB.log
    refactortestC.log
    refactortestCD.log

    • Yes
    • [ x] No

  • Is the documentation being updated? ("Documentation" includes information on the wiki or in the .rst files from doc/source/, which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/. A test build of the technical docs will be performed as part of the PR testing.)

    • Yes
    • [ x] No, does the documentation need to be updated at a later time?
      • Yes
      • [x ] No

  • Please provide any additional information or relevant details below:
    Test revealed that results after the change are bit for bit. There are test that failed while running
    1 test with dynpicard for B grid
    A number of test where namelist options for C and CD grid is not implemented.
    See attached files. base a original code. Test is new code for C, CD and B grids.
    This relate
    Test
    ./cice.setup --suite first_suite,base_suite,travis_suite,decomp_suite,reprosum_suite,quick_suite,omp_suite --mach freya --env intel,gnu --testid refactorbaseB --bgen refactorbaseB
    ./cice.setup --suite first_suite,base_suite,travis_suite,decomp_suite,reprosum_suite,quick_suite,omp_suite --mach freya --env intel,gnu --testid refactortestB --bcmp refactorbaseB
    ./cice.setup --suite first_suite,base_suite,travis_suite,decomp_suite,reprosum_suite,quick_suite,omp_suite --mach freya --env intel,gnu --testid refactorbaseC --bgen refactorbaseC -s gridc
    /cice.setup --suite first_suite,base_suite,travis_suite,decomp_suite,reprosum_suite,quick_suite,omp_suite --mach freya --env intel,gnu --testid refactortestC --bcmp refactorbaseC -s gridc
    /cice.setup --suite first_suite,base_suite,travis_suite,decomp_suite,reprosum_suite,quick_suite,omp_suite --mach freya --env intel,gnu --testid refactortestC --bcmp refactorbaseC -s gridc
    /cice.setup --suite first_suite,base_suite,travis_suite,decomp_suite,reprosum_suite,quick_suite,omp_suite --mach freya --env intel,gnu --testid refactortestC --bcmp refactorbaseC -s gridc

This relates to #755 where this is the bit for bit part and issue #739

@apcraig
Copy link
Contributor

apcraig commented Aug 22, 2022

This looks fine to me. I think this may conflict with #755, but we'll have to see. I think I'll run a full test suite on cheyenne just to verify all is bit-for-bit independently. Then I think we should merge this before #755.

@TillRasmussen
Copy link
Contributor Author

After @eclare108213 response to to #755 I decided to split the pull request in a binary identical and a none identical part. This being the identical part and #755 the none binary identical.

It is fairly simpel to handle a conflict with #755 afterwards.

@phil-blain
Copy link
Member

There are test that failed while running
1 test with dynpicard for B grid

In refactorbasetB.log and refactortestB.log I see these dynpicard failures:

FAIL freya_intel_smoke_gx3_8x1_cmplogrest_dynpicard_reprosum_run10day_thread bfbcomp freya_intel_smoke_gx3_6x4_dynpicard_reprosum_run10day different-data
FAIL freya_gnu_smoke_gx3_8x1_cmplogrest_dynpicard_reprosum_run10day_thread bfbcomp freya_gnu_smoke_gx3_6x4_dynpicard_reprosum_run10day different-data

These are expected as the VP solver is not BFB between different decompositions currently. I'm finishing up my PR that solves this.

Do we understand all the other failures?

phil-blain
phil-blain suggested changes Aug 23, 2022
View reviewed changes
Copy link
Member

@phil-blain phil-blain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Till. I agree that this is good change to make in the current state of things.

cicecore/cicedynB/dynamics/ice_dyn_evp.F90 Outdated Show resolved Hide resolved
cicecore/cicedynB/dynamics/ice_dyn_evp.F90 Outdated
Comment on lines 1166 to 1187
! U fields at NE corner
! calls ice_haloUpdate, controls bundles and masks
call dyn_HaloUpdate (halo_info, halo_info_mask, &
! U fields at NE corner
! calls ice_haloUpdate, controls bundles and masks
call dyn_HaloUpdate (halo_info, halo_info_mask, &
field_loc_NEcorner, field_type_vector, &
uvel, vvel)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same alignment fix needed here (I can't do a "Suggested change" since some lines are in the context).

cicecore/cicedynB/dynamics/ice_dyn_evp.F90 Outdated
elseif (grid_ice == "C") then
! U fields at NE corner
! calls ice_haloUpdate, controls bundles and masks
call dyn_HaloUpdate (halo_info, halo_info_mask, &
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not something new in this PR, but I'm noticing that all calls to dyn_haloUpdate in ice_dyn_evp::evp use this capitalization: dyn_HaloUpdate (with a capital H), whereas the subroutine is defined in ice_dyn_shared as dyn_haloUpdate (small h). Not that big of a deal but it could be nice to fix that .

Copy link
Contributor Author

@TillRasmussen TillRasmussen Aug 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which is preferred?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we have a firm standard or not. but for now I think we should use the case that is used when defining the subroutine (dyn_haloUpdate).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too worried about this, but happy to go the way @phil-blain suggests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @phil-blain, it's better to be consistent to make grepping just a little easier. We aren't completely consistent now but can work toward it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grep and git grep were exactly what I had in mind when mentioning it (in fact that's how I noticed!)

@apcraig
Copy link
Contributor

apcraig commented Aug 24, 2022

I have confirmed that a full test suite on cheyenne is bit-for-bit for all tests with this change, https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#d74aaac6fa8e7915a2cc7b69df57f84372df2bb9

I think this can be merged, would like @eclare108213 to have a look/review first if she has a chance.

@TillRasmussen
Copy link
Contributor Author

indents and case should be fixed. I did not redo the test

@apcraig apcraig merged commit 588a86f into CICE-Consortium:main Aug 24, 2022
@apcraig apcraig mentioned this pull request Aug 24, 2022
Merged
10 tasks
dabail10 pushed a commit to ESCOMP/CICE that referenced this pull request Oct 4, 2022
@TillRasmussen @dabail10
Refactored evp sub cycling loop (CICE-Consortium#756)
90561d9 
* Refactored evp sub cycling loop

* corrected indent and case for dyn_haloUptdate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eclare108213 eclare108213 eclare108213 approved these changes

@apcraig apcraig apcraig approved these changes

@phil-blain phil-blain phil-blain approved these changes

@JFLemieux73 JFLemieux73 Awaiting requested review from JFLemieux73

Assignees
No one assigned
Labels
Dynamics
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@TillRasmussen @apcraig @phil-blain @eclare108213